-
Couldn't load subscription status.
- Fork 2.6k
Adds friction force reporting to ContactSensor #3563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good just some docstring stuff and a slight augmentation of the test. Thanks @gattra-rai
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor.py
Outdated
Show resolved
Hide resolved
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor.py
Outdated
Show resolved
Hide resolved
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor.py
Outdated
Show resolved
Hide resolved
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I like how you refactor and docstring the unpack
|
I am curious how do you use the friction force for your learning setup, would love to learn how you guys are using it : )) |
a08d71c to
1fbdb77
Compare
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor.py
Outdated
Show resolved
Hide resolved
| * If the :attr:`ContactSensorCfg.track_friction_forces` is False, then this quantity is None. | ||
| * If the :attr:`ContactSensorCfg.filter_prim_paths_expr` is empty, then this quantity is an empty tensor. | ||
| * If the :attr:`ContactSensorCfg.max_contact_data_per_prim` is not specified or less than 1, then this quantity | ||
| will not be calculated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three conditions are mentioned but in the code, the checks are only performed when track_friction_forces are provided. Is this intentional? Do we handle the cases where the configuration is provided incorrectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this is not the net contact forces, right? Just making sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've added a validation check per gremlin's recommendation, in which a value error is raised if track_friction_forces = True but either filter_prim_paths_expr = None | [] or max_contact_data_per_prim = None | < 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and yes this is not the net contact forces
| expected_friction, _, _, _ = scene["contact_sensor"].contact_physx_view.get_friction_data(dt=sim_dt) | ||
| reported_friction = scene["contact_sensor"].data.friction_forces_w[0, 0, :] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of comparing the two to the same PhysX buffers, it would make more sense to compare the physical realism of this. For example, if the cube is on a ground with friction of coefficient as 1 and we add a push velocity, then the friction force is mu * Mg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my thinking was that we shouldn't be testing the underlying functionality of the physx API, and instead just test that the friction force reporting code is doing what is intended, which is to provide the unpacked friction tensor from the physx view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would also require reverse engineering the physx friction force implementation, which could change and break the test, despite the code under test not being broken
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor.py
Outdated
Show resolved
Hide resolved
|
@greptileai some help ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happy to help! Could you please clarify what you need assistance with regarding this PR?
For example, are you asking about:
- How the friction force tracking implementation works?
- Understanding specific parts of the code changes?
- Testing the new feature?
- Integration with existing code?
- Performance considerations?
- Something else entirely?
Let me know what specific aspect you'd like me to explain or help with! 😊
|
@greptileai everything you said. Whatever it takes to improve the MR and make it less error-prone and feature complete :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this PR, I have several suggestions to improve robustness and completeness:
1. Add validation in _initialize_impl
The track_friction_forces feature requires filter_prim_paths_expr to be set and max_contact_data_count_per_prim > 0, just like track_contact_points. Add validation:
# check if filter paths are valid
if self.cfg.track_contact_points or self.cfg.track_friction_forces:
if len(self.cfg.filter_prim_paths_expr) == 0:
raise ValueError(
f"The 'filter_prim_paths_expr' is empty. Please specify a valid filter pattern to track"
f" {'contact points' if self.cfg.track_contact_points else 'friction forces'}."
)
if self.cfg.max_contact_data_count_per_prim < 1:
raise ValueError(
f"The 'max_contact_data_count_per_prim' is {self.cfg.max_contact_data_count_per_prim}. "
f"Please set it to a value greater than 0 to track"
f" {'contact points' if self.cfg.track_contact_points else 'friction forces'}."
)
2. Fix reset logic for friction forces
In the reset method, you're resetting friction_forces_w to 0.0, but for contact_pos_w you use torch.nan. The default in _unpack_contact_data for friction is 0.0, so this is consistent. However, consider whether 0.0 is the right default vs. NaN to distinguish "no contact" from "zero friction force":
# reset contact positions
if self.cfg.track_contact_points:
self._data.contact_pos_w[env_ids, :] = torch.nan
# reset friction forces
if self.cfg.track_friction_forces:
self._data.friction_forces_w[env_ids, :] = 0.0
Consider: Should friction forces also use NaN for no-contact cases? This would be more consistent with contact_pos_w. You'd need to update the default parameter and the test assertions.
3. Add bounds checking in _unpack_contact_data
The vectorized unpacking could fail with invalid indices. Add safety checks:
def _unpack_contact_data(
self,
contact_data: torch.Tensor,
buffer_count: torch.Tensor,
buffer_start_indices: torch.Tensor,
avg: bool = True,
default: float = float("nan"),
) -> torch.Tensor:
"""
Unpacks and aggregates contact data for each (env, body, filter) group.
This function vectorizes the following nested loop:
for i in range(self._num_bodies * self._num_envs):
for j in range(self.contact_physx_view.filter_count):
start_index_ij = buffer_start_indices[i, j]
count_ij = buffer_count[i, j]
self._contact_position_aggregate_buffer[i, j, :] = torch.mean(
contact_data[start_index_ij : (start_index_ij + count_ij), :], dim=0
)
For more details, see the RigidContactView.get_contact_data() documentation:
https://docs.omniverse.nvidia.com/kit/docs/omni_physics/107.3/extensions/runtime/source/omni.physics.tensors/docs/api/python.html#omni.physics.tensors.impl.api.RigidContactView.get_net_contact_forces
Args:
contact_data (torch.Tensor): Flat tensor of contact data, shape (N_envs * N_bodies, 3).
buffer_count (torch.Tensor): Number of contact points per (env, body, filter), shape (N_envs * N_bodies, N_filters).
buffer_start_indices (torch.Tensor): Start indices for each (env, body, filter), shape (N_envs * N_bodies, N_filters).
avg (bool, optional): If True, average the contact data for each group; if False, sum the data. Defaults to True.
default (float, optional): Default value to use for groups with zero contacts. Defaults to NaN.
Returns:
torch.Tensor: Aggregated contact data, shape (N_envs, N_bodies, N_filters, 3).
"""
counts, starts = buffer_count.view(-1), buffer_start_indices.view(-1)
n_rows, total = counts.numel(), int(counts.sum())
agg = torch.full((n_rows, 3), default, device=self._device, dtype=contact_data.dtype)
if total > 0:
row_ids = torch.repeat_interleave(torch.arange(n_rows, device=self._device), counts)
block_starts = counts.cumsum(0) - counts
deltas = torch.arange(row_ids.numel(), device=counts.device) - block_starts.repeat_interleave(counts)
flat_idx = starts[row_ids] + deltas
pts = contact_data.index_select(0, flat_idx)
agg = agg.zero_().index_add_(0, row_ids, pts)
if avg:
agg = agg / counts.clamp_min(1).unsqueeze(-1)
agg[counts == 0] = default
return agg.view(self._num_envs * self.num_bodies, -1, 3).view(
self._num_envs, self._num_bodies, self.contact_physx_view.filter_count, 3
)
4. Improve documentation
The docstring for friction_forces_w mentions "Average" but it's actually a sum when avg=False. Clarify:
friction_forces_w: torch.Tensor | None = None
"""Sum of friction forces between sensor body and filter prim in world frame.
Shape is (N, B, M, 3), where N is the number of sensors, B is number of bodies in each sensor
and M is the number of filtered bodies.
Collision pairs not in contact will have zero friction forces.
Note:
* If the :attr:`ContactSensorCfg.track_friction_forces` is False, then this quantity is None.
* If the :attr:`ContactSensorCfg.filter_prim_paths_expr` is empty, then this quantity is an empty tensor.
* If the :attr:`ContactSensorCfg.max_contact_data_per_prim` is not specified or less than 1, then this quantity
will not be calculated.
"""
5. Add edge case test
Add a test for when filter_prim_paths_expr is empty but track_friction_forces=True to verify the error handling.
These changes will make the feature more robust and easier to understand!
|
Responding to greptile's suggestions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This review covers only the latest changes addressing previous feedback. The PR adds friction force tracking to ContactSensor by introducing a configuration toggle and data field. Recent changes include fixing test assertion issues by replacing division with floor division for shape calculations, removing a duplicate sensor update call in the check script, improving documentation formatting, setting friction force defaults to 0.0 instead of NaN for non-contact cases, adding type hints to the helper method, and formatting docstrings to follow Google style. The implementation refactors the contact data unpacking into a reusable helper method that serves both contact points and friction forces tracking.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor.py | 5/5 | Adds friction force tracking with proper validation, refactors unpacking logic into helper method with complete type hints and Google-style docstrings |
| source/isaaclab/test/sensors/test_contact_sensor.py | 5/5 | Fixes critical shape assertion using floor division, adds friction force validation tests including physical realism checks |
| source/isaaclab/test/sensors/check_contact_sensor.py | 5/5 | Removes duplicate sensor update call that was affecting performance benchmarks |
| source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor_data.py | 4/5 | Adds friction_forces_w field with improved documentation, minor formatting inconsistency in bullet point indentation |
| source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor_cfg.py | 5/5 | Adds track_friction_forces configuration flag following existing patterns |
| source/isaaclab/docs/CHANGELOG.rst | 5/5 | Documents new friction tracking feature in version 0.47.2 with proper formatting |
| source/isaaclab/config/extension.toml | 5/5 | Bumps version to 0.47.2 for new feature |
| CONTRIBUTORS.md | 5/5 | Adds Greg Attra to contributors list in alphabetical order |
Confidence score: 4/5
- This PR is safe to merge with minor documentation polish recommended but no blocking issues
- Score reflects that all critical technical issues from previous reviews have been addressed: division operator fixed, duplicate update removed, type hints added, docstrings formatted correctly, and default values set appropriately; minor deduction only for documentation formatting inconsistency
- Pay close attention to source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor_data.py for the bullet point indentation issue on lines 41 and 58
Additional Comments (1)
-
source/isaaclab/test/sensors/check_contact_sensor.py, line 178 (link)logic: duplicate
update()call - the sensor is already updated in the timed block above (line 175)
8 files reviewed, 8 comments
| friction_forces, _, buffer_count, buffer_start_indices = sensor.contact_physx_view.get_friction_data( | ||
| dt=sensor._sim_physics_dt | ||
| ) | ||
| for i in range(sensor.num_instances * num_bodies): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: potential type mismatch: num_instances * num_bodies may need explicit int conversion if num_instances is float from line 706
| start_index_ij = buffer_start_indices[i, j] | ||
| count_ij = buffer_count[i, j] | ||
| force = torch.sum(friction_forces[start_index_ij : (start_index_ij + count_ij), :], dim=0) | ||
| assert torch.allclose(force, sensor._data.friction_forces_w[i, j, :], atol=1e-5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: indexing mismatch: friction_forces_w[i, j, :] has 3 dimensions but should be friction_forces_w[i, j, 0, :] based on the shape (N, num_bodies, 1, 3)
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor_data.py
Outdated
Show resolved
Hide resolved
source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor_data.py
Outdated
Show resolved
Hide resolved
| if self.cfg.track_contact_points or self.cfg.track_friction_forces: | ||
| if len(self.cfg.filter_prim_paths_expr) == 0: | ||
| raise ValueError( | ||
| "The 'filter_prim_paths_expr' is empty. Please specify a valid filter pattern to track" | ||
| f" {'contact points' if self.cfg.track_contact_points else 'friction forces'}." | ||
| ) | ||
| if self.cfg.max_contact_data_count_per_prim < 1: | ||
| raise ValueError( | ||
| f"The 'max_contact_data_count_per_prim' is {self.cfg.max_contact_data_count_per_prim}. " | ||
| "Please set it to a value greater than 0 to track" | ||
| f" {'contact points' if self.cfg.track_contact_points else 'friction forces'}." | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: validation logic checks track_contact_points OR track_friction_forces but the error message only mentions one feature at a time. If both are enabled, the message might be misleading. Should the error message indicate which specific feature(s) triggered the validation failure when both are enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This review covers only the latest changes made since the last review, not the entire PR. The most recent update addresses two critical syntax errors in the test file where standard division (/) was mistakenly used instead of integer division (//) when calculating tensor shapes for validation. In test_contact_sensor.py, lines 706 and738 now correctly use // to ensure integer results when dividing num_instances by num_bodies, which is essential for proper shape comparison in PyTorch tensors. Additionally, the data class documentation formatting has been corrected to ensure consistent indentation for validation rule bullet points.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sensors/contact_sensor/contact_sensor_data.py | 5/5 | Minor documentation formatting fix for validation rule indentation |
| source/isaaclab/test/sensors/test_contact_sensor.py | 5/5 | Critical syntax fix: changed / to // for integer division in shape validation |
Confidence score: 5/5
- This PR is safe to merge with minimal risk
- Both changes are straightforward fixes: one corrects documentation formatting and the other fixes a critical type error in test validation logic that would have caused test failures
- No files require special attention as both changes are simple, necessary corrections
2 files reviewed, no comments
Description
This PR extends the
ContactSensorclass to expose aggregated friction forces for each filtered body. It uses the same vectorized approach used forcontact_points.Concretely, this change introduces:
ContactSensorCfg.track_friction_forcestoggle to turn on friction trackingContactSensorData.friction_forces_wwhere the sum of friction forces for each filtered body are storedFixes #2074, #2064
Performance
Results of
check_contact_sensor.pywithtrack_friction_data = False:Results of
check_contact_sensor.pywithtrack_friction_data = True:Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there